Skip to content
This repository has been archived by the owner on Dec 2, 2024. It is now read-only.

SCP-3305 SCP-3263 fixed Ledger.Constraints.Offchain.updateUtxoIndex #275

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

bwbush
Copy link
Collaborator

@bwbush bwbush commented Jan 24, 2022

Prior to this commit, Ledger.Constraints.Offchain.updateUtxoIndex would discard lookups for inputs that aren't script outputs. However, sometimes it is necessary to include particular public-key TxIns in a transaction, via Plutus.Contract.Wallet.ExportTx.lookups: a good example of this is when a Plutus.Contract.Currency.OneShotCurrency needs to consume a specified UTxO in its minting policy. Even though Plutus.Constraints.Tx.mustSpendPubKeyOutput adds a public-key input to the lookups, updateUtxoIndex discards that, with the result that it is only by chance that cardano-wallet would select that necessary input for the balanced transaction and provide it to Plutus validators. (Note that cardano-wallet does not automatically provide the TxIn in the partially constructed transaction to validators: it only provides Cardano.Wallet.PartialTx.inputs and the inputs it has semi-randomly chosen.) The consequence of all of this is that scripts that require a particular input will randomly fail during balancing in wallets with more than one UTxO.

This fix simply adds public-key inputs to the type Ledger.Constraints.OffChain.ScriptOutput so that it can also hold public-key inputs. (There might be a more descriptive name than ScriptOutput now.) It makes minor adjustments to the chain of functions that pass this into an unbalanced transaction before it is sent to cardano-wallet.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, materialized Nix files, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@bwbush bwbush requested a review from raduom January 24, 2022 18:29
@bwbush bwbush self-assigned this Jan 24, 2022
@bwbush bwbush marked this pull request as ready for review January 24, 2022 18:49
@raduom raduom requested a review from sjoerdvisscher January 25, 2022 17:22
Copy link
Contributor

@raduom raduom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I have not worked in this area recently, so I asked @sjoerdvisscher for a review also.

@sjoerdvisscher
Copy link
Contributor

The reason we removed public-key inputs is that cardano-wallet doesn't support it. See #57.
So it might be that we were wrongly informed, but did you check that this works with cardano-wallet?

Copy link
Contributor

@sjoerdvisscher sjoerdvisscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, the way to go would be to revert #57.

@bwbush
Copy link
Collaborator Author

bwbush commented Jan 26, 2022

The reason we removed public-key inputs is that cardano-wallet doesn't support it. See #57. So it might be that we were wrongly informed, but did you check that this works with cardano-wallet?

Yes, I've tested this will wallet and actual Marlowe contract endpoints. The MustSpendPubKeyOutput could have been named MustSpendWalletOutput because it just provides a UTxO that could reside at any of the wallet's addresses.

I stepped through each phase of wallet's processing of the request from the PAB to see how it balances the transaction: the wallet makes provision for spending particular UTxOs in the PartialTx.inputs, which is poplulated from ExportTx. The wallet only provides UTxOs to scripts if they are specified in PartialTx.inputs or if the wallet happens to select the UTxO when it balances; unfortunately, the wallet does not pass the TxIns contained in PartialTx.partialTx :: Tx AlonzoEra to scripts involved in the transaction.

@bwbush
Copy link
Collaborator Author

bwbush commented Jan 26, 2022

If we do this, the way to go would be to revert #57.

It looks like reverting #57 would work, but I'd like to test that.

@bwbush bwbush marked this pull request as draft January 27, 2022 16:56
@bwbush bwbush force-pushed the marlowe/SCP-3305a branch 2 times, most recently from 696c043 to 4d0869e Compare January 27, 2022 19:31
@bwbush bwbush force-pushed the marlowe/SCP-3305a branch from 4d0869e to d08145b Compare January 27, 2022 20:02
@bwbush bwbush marked this pull request as ready for review January 27, 2022 20:20
@bwbush
Copy link
Collaborator Author

bwbush commented Jan 27, 2022

@sjoerdvisscher, I reverted #57 in d08145b. The Marlowe PAB integration test passes (running the full Marlowe contract on our private testnet) using this revision. Would you please re-review? Thanks.

@bwbush bwbush requested a review from sjoerdvisscher January 27, 2022 20:20
Copy link
Contributor

@sjoerdvisscher sjoerdvisscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like all this red :)

@bwbush bwbush merged commit ea1bfc6 into main Jan 28, 2022
@bwbush bwbush deleted the marlowe/SCP-3305a branch January 28, 2022 12:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants